Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

New Widget: Audio #1506

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

New Widget: Audio #1506

wants to merge 2 commits into from
+431 −2

Conversation

bartbutenaers
Copy link
Contributor

@bartbutenaers bartbutenaers commented Nov 29, 2024 •
edited
Loading

Description

This PR adds a new ui-audio widget to the palette, which can be used to play dynamically audio files in the dashboard D2. Similar like the audio-out node of dashboard 1.

I marked this PR as draft, because it is not fully completed. I need some feedback to complete it. Until then I won't be working on this PR, so would be nice if somebody could assist me so I can finalize this PR. Thanks!

This is how it looks in my Chrome browser:

image

Example flow to demonstrate that all the properties can be dynamically overwritten via inpute messages:

[{"id":"3c290f1ea6cf8b36","type":"inject","z":"4aad778b57d4f47b","name":"Pause","props":[{"p":"playback","v":"pause","vt":"str"}],"repeat":"","crontab":"","once":false,"onceDelay":0.1,"topic":"","x":1890,"y":3540,"wires":[["b6d975ba7cfabfcd"]]},{"id":"436efb1217534529","type":"inject","z":"4aad778b57d4f47b","name":"Stop","props":[{"p":"playback","v":"stop","vt":"str"}],"repeat":"","crontab":"","once":false,"onceDelay":0.1,"topic":"","x":1890,"y":3500,"wires":[["b6d975ba7cfabfcd"]]},{"id":"bfc886482dbdb38c","type":"debug","z":"4aad778b57d4f47b","name":"Audio output","active":true,"tosidebar":true,"console":false,"tostatus":false,"complete":"true","targetType":"full","statusVal":"","statusType":"auto","x":2250,"y":3140,"wires":[]},{"id":"b6d975ba7cfabfcd","type":"ui-audio","z":"4aad778b57d4f47b","group":"1e2acdd1dfa22bf9","name":"","order":0,"width":"6","height":"1","src":"","autoplay":"off","controls":"show","loop":"off","muted":"off","x":2080,"y":3140,"wires":[["bfc886482dbdb38c"]]},{"id":"f878114e50dace0a","type":"inject","z":"4aad778b57d4f47b","name":"Enable","props":[{"p":"enabled","v":"true","vt":"bool"}],"repeat":"","crontab":"","once":false,"onceDelay":0.1,"topic":"","x":1890,"y":2920,"wires":[["b6d975ba7cfabfcd"]]},{"id":"e5b91f1f4fb4fd55","type":"inject","z":"4aad778b57d4f47b","name":"Muted off","props":[{"p":"ui_update.muted","v":"off","vt":"str"}],"repeat":"","crontab":"","once":false,"onceDelay":0.1,"topic":"","x":1880,"y":3180,"wires":[["b6d975ba7cfabfcd"]]},{"id":"59303ca2418166c2","type":"inject","z":"4aad778b57d4f47b","name":"Muted on","props":[{"p":"ui_update.muted","v":"on","vt":"str"}],"repeat":"","crontab":"","once":false,"onceDelay":0.1,"topic":"","x":1880,"y":3140,"wires":[["b6d975ba7cfabfcd"]]},{"id":"03afd1a13fc70794","type":"inject","z":"4aad778b57d4f47b","name":"Loop off","props":[{"p":"ui_update.loop","v":"off","vt":"str"}],"repeat":"","crontab":"","once":false,"onceDelay":0.1,"topic":"","x":1890,"y":3260,"wires":[["b6d975ba7cfabfcd"]]},{"id":"c340c61db669fc1f","type":"inject","z":"4aad778b57d4f47b","name":"Loop on","props":[{"p":"ui_update.loop","v":"on","vt":"str"}],"repeat":"","crontab":"","once":false,"onceDelay":0.1,"topic":"","x":1880,"y":3220,"wires":[["b6d975ba7cfabfcd"]]},{"id":"900f167dee26d78b","type":"inject","z":"4aad778b57d4f47b","name":"Autoplay on","props":[{"p":"ui_update.autoplay","v":"on","vt":"str"}],"repeat":"","crontab":"","once":false,"onceDelay":0.1,"topic":"","x":1870,"y":3300,"wires":[["b6d975ba7cfabfcd"]]},{"id":"f4c14f571d72b9e6","type":"inject","z":"4aad778b57d4f47b","name":"Autoplay off","props":[{"p":"ui_update.autoplay","v":"off","vt":"str"}],"repeat":"","crontab":"","once":false,"onceDelay":0.1,"topic":"","x":1870,"y":3340,"wires":[["b6d975ba7cfabfcd"]]},{"id":"a516fd15f0df3401","type":"inject","z":"4aad778b57d4f47b","name":"Src Friends","props":[{"p":"payload"}],"repeat":"","crontab":"","once":false,"onceDelay":0.1,"topic":"","payload":"http://www.sitcomsonline.com/sounds/friends1996.wav","payloadType":"str","x":1870,"y":3380,"wires":[["b6d975ba7cfabfcd"]]},{"id":"b89a1933c291bc6b","type":"inject","z":"4aad778b57d4f47b","name":"Src StarWars","props":[{"p":"payload"}],"repeat":"","crontab":"","once":false,"onceDelay":0.1,"topic":"","payload":"https://www2.cs.uic.edu/~i101/SoundFiles/ImperialMarch60.wav","payloadType":"str","x":1870,"y":3420,"wires":[["b6d975ba7cfabfcd"]]},{"id":"4fbc55f25609fb08","type":"inject","z":"4aad778b57d4f47b","name":"Play","props":[{"p":"playback","v":"play","vt":"str"}],"repeat":"","crontab":"","once":false,"onceDelay":0.1,"topic":"","x":1890,"y":3460,"wires":[["b6d975ba7cfabfcd"]]},{"id":"b737027f04ea228f","type":"inject","z":"4aad778b57d4f47b","name":"Disable","props":[{"p":"enabled","v":"false","vt":"bool"}],"repeat":"","crontab":"","once":false,"onceDelay":0.1,"topic":"","x":1890,"y":2960,"wires":[["b6d975ba7cfabfcd"]]},{"id":"80bc186add2ddefc","type":"inject","z":"4aad778b57d4f47b","name":"Visible","props":[{"p":"visible","v":"true","vt":"bool"}],"repeat":"","crontab":"","once":false,"onceDelay":0.1,"topic":"","x":1890,"y":3000,"wires":[["b6d975ba7cfabfcd"]]},{"id":"73a1c76f72ec86f2","type":"inject","z":"4aad778b57d4f47b","name":"Invisible","props":[{"p":"visible","v":"false","vt":"bool"}],"repeat":"","crontab":"","once":false,"onceDelay":0.1,"topic":"","x":1880,"y":3040,"wires":[["b6d975ba7cfabfcd"]]},{"id":"1e2acdd1dfa22bf9","type":"ui-group","name":"Experiments","page":"e06eacf47a88bb87","width":"6","height":"1","order":1,"showTitle":true,"className":"","visible":"true","disabled":"false"},{"id":"e06eacf47a88bb87","type":"ui-page","name":"Alarm","ui":"be29745a6e568f30","path":"/alarm","icon":"lock","layout":"grid","theme":"092547d34959327c","breakpoints":[{"name":"Default","px":0,"cols":3},{"name":"Tablet","px":576,"cols":6},{"name":"Small Desktop","px":768,"cols":9},{"name":"Desktop","px":1024,"cols":12}],"order":7,"className":"","visible":true,"disabled":false},{"id":"be29745a6e568f30","type":"ui-base","name":"Node-RED","path":"/dashboard","appIcon":"","includeClientData":true,"acceptsClientConfig":["ui-notification","ui-control","ui-chart","ui-text-input","ui-dropdown"],"showPathInSidebar":false,"showPageTitle":true,"navigationStyle":"icon","titleBarStyle":"default"},{"id":"092547d34959327c","type":"ui-theme","name":"Theme Name","colors":{"surface":"#ffffff","primary":"#0094ce","bgPage":"#eeeeee","groupBg":"#ffffff","groupOutline":"#cccccc"}}]

image

Open issues:

  1. When e.g. a "play" command is send to this node, it could fail. For example when there is no src url set, or when the user hasn't executed any manual actions yet on the page (because browsers don't like sites to start playing audio automatically). Which is normal, but you only see a message in the browser console. Which might be confusing for users to understand why it doesn't play. Should I send those errors as output messages? If yes, which format should those messages have?
  2. Currently the audio element is displayed on a specified group. As a result the audio will stop playing if you navigate to another tab in the dashboard. Not sure if that is wanted behaviour, or how I could fix that (if not wanted).
  3. A html audio element has a controls property which you can use to show or hide the controls (i.e. play button, and so on...). I first had supported that, but if you don't show the controls you simply get an empty area which is quite useless. So I thought it would better if you could enable or hide the ui-audio widget via the dashboard build-in features msg.visible and msg.enabled. But those don't work (see example flow above). No idea whether I need to do something special for that.
  4. I have not added a unit test. Although I know that is important, I have to be honest that my limited free time doesn't allow me to start reading about how to do that. So would be nice if anybody else could add that.
  5. The documentation md file is near complete but there should be also a nice image of element and a demo flow. Again I would appreciate if somebody else could add those.

Related Issue(s)

52

Checklist

  • I have read the contribution guidelines
  • Suitable unit/system level tests have been added and they pass
  • Documentation has been updated
    • Upgrade instructions
    • Configuration details
    • Concepts
  • Changes flowforge.yml?
    • Issue/PR raised on FlowFuse/helm to update ConfigMap Template
    • Issue/PR raised on FlowFuse/CloudProject to update values for Staging/Production

Labels

  • Includes a DB migration? -> add the area:migration label

@joepavitt joepavitt self-requested a review November 29, 2024 16:53
@joepavitt
Copy link
Collaborator

Which might be confusing for users to understand why it doesn't play. Should I send those errors as output messages? If yes, which format should those messages have?

I think in the case of a missing src, that's something we can check server-side and report into the Node-RED debug panel?

Currently the audio element is displayed on a specified group.

As a first iteration, this is sensible to me

A html audio element has a controls property which you can use to show or hide the controls (i.e. play button, and so on...). I first had supported that, but if you don't show the controls you simply get an empty area which is quite useless. So I thought it would better if you could enable or hide the ui-audio widget via the dashboard build-in features msg.visible and msg.enabled. But those don't work (see example flow above). No idea whether I need to do something special for that.

Let's leave for first pass and worry about that another time, having to see controls is a very good first iteration

I have not added a unit test. Although I know that is important, I have to be honest that my limited free time doesn't allow me to start reading about how to do that. So would be nice if anybody else could add that.

We can add, I'm not going to force people to write E2E tests n their free time 😄 Manual testing will suffice for now

The documentation md file is near complete but there should be also a nice image of element and a demo flow. Again I would appreciate if somebody else could add those.

Will take a look as part of the review once we've addressed 1.

@joepavitt joepavitt changed the title New ui-audio widget New Widget: Audio Dec 2, 2024
@bartbutenaers
Copy link
Contributor Author

I think in the case of a missing src, that's something we can check server-side and report into the Node-RED debug panel?

@joepavitt Yes but I was wondering if there is perhaps a standard way already available for nodes to report client-side errors to the server-side? Something like:

this.$socket.emit('widget-action', this.id, msg)

I don't think it exist yet, but it would be kind of silly if I create a workaround specific for this single one node?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joepavitt joepavitt Awaiting requested review from joepavitt

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bartbutenaers @joepavitt